[FIX] common: close database connections, fix helpers, isolate the test suite - #178

…st suite (#178)

## Why

This started as filling coverage gaps, and each step surfaced the next:

1. `.coveragerc` gates at 60% and the suite sat at **64%**, with the gap widest on pure logic the rest of the framework leans on. Writing those tests surfaced **six defects in the helpers** — without fixing them the tests would have pinned broken behaviour.
2. Verifying the fixes meant running the suite repeatedly, which is when it became clear that **two suites cannot run at once**: identical invocations produced anywhere from 0 to 68 failures, and the machine had accumulated 16 orphaned `/tmp/odev-test-*` directories.
3. Isolating the runs revealed that three concurrent suites exhaust PostgreSQL's connection slots, which turned out to be a **connection leak in odev itself**, not in the suite.

The four sections below are independent and the commits are ordered to be reviewed in sequence.

## 1. Helper defects — `42569c4`

| Location | Defect |
|---|---|
| `connectors/postgres.py` `columns_exist` | Returned `[]` when **none** of the requested columns existed — indistinguishable from all being present. `CREATE TABLE IF NOT EXISTS` leaves an existing table alone, so the missing-columns pass is the only thing that can migrate a table created from an older definition; it silently added nothing. |
| `postgres.py` `PostgresDatabase.tables` | A class attribute, so every instance shared one registry and tables from different databases collided on their name alone. |
| `string.py` `quote` | Chose its delimiter with `max()` over both quote offsets, picking the **last** rather than the first, mis-quoting any string mixing them. |
| `version.py` `OdooVersion.__bool__` | Always `True` — `module` is padded to `MIN_VERSION_LENGTH` and is never an empty tuple. |
| `string.py` `min_indent` | Raised `ValueError` on a text without any non-blank line, reachable from `odev help` through `dedent`. |
| `float_to_hours`, `strip_styles` | Broken, but called nowhere in odev nor in the plugins. **Left alone**, documented in the tests with the correction spelled out. |

`columns_exist` has exactly one caller, and it runs after `CREATE TABLE IF NOT EXISTS`, so the fix cannot make it issue `ALTER TABLE` against a missing table.

## 2. Coverage for the untested helpers — `be95197`

- **`test_string.py`** (new) — `string.py` had no test module at all, despite backing `odev help`, `odev history` and the local database listing query. Sizes and their round-trip, indentation, joining, the `dirty_only` × `force_single` quoting matrix, Rich markup helpers, and the `help` column alignment contract.
- **`test_git_worktree.py`** (new) — `connectors/git.py` was the least-covered large module (34%), and its `GitWorktree` parser turns `git worktree list --porcelain` into the objects the whole `fetch` / `pull` / `worktree` family works with. Porcelain parsing (branch, detached, bare, locked, prunable with reasons), the `-odev-` local-branch split that `create_worktree` writes and `fetch` / `pull` read back, identity by path, and `pending_changes` including the two swallowed `GitCommandError` messages. No network, no real repository.
- **`test_postgres_table.py`** (new) — `PostgresTable.__add_missing_column`, the datastore's migration path, was entirely unreached; this covers it including the `InvalidTableDefinition` primary-key branch.
- **`test_version.py`** — ordering (`15.0 < 16.0 < saas-16.4 < 17.0 < master`) is what actually picks a revision at runtime and nothing compared two versions.

Corrections to existing tests, in the same commit:

- **`test_bash.py` shelled out to a real `sudo cat >> /etc/shadow`.** The premise that the command fails only holds for an unprivileged user whose shell cannot open the redirection — a machine granting passwordless sudo runs it for real, and as root it appends to the file or hangs on stdin. The subprocess and the effective user are now simulated, which also lets the elevation path be asserted rather than inferred.
- `test_odev.py` left a command line behind in `sys.argv` for whichever test ran next.
- `tests/fixtures/case.py` — `_patches` was a list defined on `OdevTestCase` and mutated through `cls._patches.append`, so every subclass shared it and each class tore down the patches of all the classes before it.

## 3. An isolated, self-cleaning test suite — `87dc0e4`, `86630d3`, `716c8e1`

`Odev.name` was the constant `"odev-test"` and **every** shared resource derived from it, so two suites shared one namespace and actively destroyed each other:

- `test_99_delete_expression` ran `odev delete --expression "^odev-test-[a-z0-9]{8}" --include-whitelisted` against the real PostgreSQL, deleting a concurrent run's databases.
- `PostgresDatabase.drop()` terminates every backend on `datname`, so each class teardown killed a concurrent run's cursors.
- `CREATE TABLE IF NOT EXISTS` is not atomic, and `Config.save()` truncate-writes a fixed path — hence `UniqueViolation` on `pg_type_typname_nsp_index` and `DuplicateOptionError` from a torn config.

A run now claims a sandbox named after itself and holds an exclusive `flock` on it for its whole life. Everything — datastore, test databases, config, temp directories — is named after it or nested under it. Cleanup runs at `pytest_sessionfinish`, which pytest calls from a `finally`, so `Ctrl+C` is covered; `SIGTERM` becomes the same orderly exit; and the next run's sweep collects whatever a `SIGKILL` left, because the kernel releases the lock when the owner dies whatever the cause.

**The suite was also writing outside its sandbox**, which is worth a look on its own:

- `TestSetup` ran the install scripts against their real destinations, so running the suite **repointed the developer's `~/.local/bin/odev` and bash-completion symlinks at whichever checkout it ran from**. `symlink.py` computed the destination halfway through creating it, leaving no way to redirect it; that decision moves to `link_path`.
- Tests cloned into the real `~/odoo/repositories`. The repositories, dumps and upgrade paths now point inside the sandbox, as does `CONFIG_DIR` — which also means the suite no longer picks up whichever plugins the developer happens to have installed, so a local run and CI exercise the same code.

`87dc0e4` is a separate product fix this surfaced: `LocalDatabase.is_odoo` checks that a database exists and then connects to it, and any process can drop it in between — `odev list` inspects every database in turn and would fail outright because one went away.

Interrupt handling is covered by `tests/tests/common/test_interrupts.py`: odev captures `SIGINT` around every query to cancel just that statement, so a `Ctrl+C` was previously swallowed and the run carried on. Letting it through instead abandons the connection mid-statement, so the interrupt is recorded and acted upon at the next test boundary.

## 4. Connection lifetime — `47499cb`, `cd07007`

Both database context managers built a **second, unconnected** connector to close instead of the one they had connected, so `disconnect()` did nothing and the connection stayed open until the garbage collector got to it:

```python
def __enter__(self):
    self.connector = self._connector_class(self.name).__enter__()   # connector A, connected
    return self

def __exit__(self, *args):
    self._connector_class(self.name).__exit__(*args)                # connector B, never connected
```

`ensure_connected` runs every database method inside its own block and those blocks nest — `is_odoo` opens one and then calls `table_exists`, which opens another — so this meant a fresh backend per call.

Closing the right connector is **not enough on its own**: an inner block would close the connection the enclosing one is still using. The blocks are now reentrant and share a single connector, counted in `PostgresConnectorMixin` so both classes get the same behaviour. The datastore holds its connection instead of reopening it per read — every command reads it and it lives as long as the process, which is not true of the databases odev walks through for `list` or `delete`.

A connection pool keyed per database was considered and set aside: `list --all` and `delete --expression` touch **every** database on the server briefly, so a per-database pool would hold one idle backend per Odoo database until the process ends — the very exhaustion this fixes — unless it also grew a global cap and idle eviction.

Measured over a full suite run:

| | before | after |
|---|---|---|
| peak backends held | 42 | **3** |
| mean backends held | 8.7 | **0.8** |
| suite duration | 54.8s | **33.6s** |
| three concurrent suites | died on `max_connections` | **249 passed each**, peak 7 backends |

The speedup was not the goal — it is what a backend fork plus an authentication round-trip per query costs.

## Coverage

| Module | Before | After |
|---|---|---|
| `common/string.py` | 85% | **100%** |
| `common/version.py` | 96% | **100%** |
| `common/postgres.py` | 81% | **93%** |
| `common/connectors/git.py` | 34% | **40%** |
| **Total** | **64%** | **65%** |

## Verification

- `pytest tests` — **249 passed**, from 242 on the first revision
- Two and three concurrent suites — **249 passed each**, repeatedly, leaving zero directories and zero databases behind
- `SIGINT`, `SIGTERM` and `SIGKILL` mid-run — each verified to leave nothing behind, the last one via the next run's sweep
- `odev list --all`, `odev history`, `odev version` — smoke-checked, no connections surviving the process
- `pre-commit run --all-files` — clean
- `basedpyright` — 3 errors, all pre-existing on `beta`; **0 new**

## Notes for reviewers

- `odev/_version.py` is bumped once, to `4.29.10`. `origin/beta` is at `4.29.9`; PRs #175, #176 and #177 each bump from the same base, so whichever merges second needs a one-line rebase.
- `LocalDatabase.connector: PostgresConnector | None = None` was removed as dead — `ConnectorMixin.__init__` overwrites it with the connector *class* at construction, which also meant the `if self.connector is not None` guard in `_restore` never protected anything. It is now the `isinstance` check `drop()` already used.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

https://claude.ai/code/session_012tWyDuUYenE92nsBtG19Du